Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Updated proposal: Use moment defaults and preserve global settings. #257

Closed
wants to merge 2 commits into from
Closed

Updated proposal: Use moment defaults and preserve global settings. #257

wants to merge 2 commits into from

Conversation

martijnrusschen
Copy link
Member

Updated version of #228
Should fix #178

@martijnrusschen
Copy link
Member Author

We need better tests.. Build passes, but examples are failing.

@rafeememon
Copy link
Contributor

I have high hopes that this might also fix #160

@martijnrusschen
Copy link
Member Author

Care to look into this? I ported the changes from #228, but it doesn't work for me.

@rafeememon
Copy link
Contributor

I'll be traveling over the next week or so, but I can take a crack at it after that!

rafeememon
rafeememon reviewed Dec 17, 2015
View reviewed changes
src/calendar.jsx
weekdaysMin: weekdays
});
if (this.props.locale) { thisMoment.locale(this.props.locale); }
if (this.props.weekStart) { thisMoment._locale._week.dow = this.props.weekStart; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this line is problematic. Modifying _locale touches the global locale, which we want to avoid. I found this out by trying to change _locale._weekdaysMin in one picker and seeing that it affected all pickers.

@rafeememon
Copy link
Contributor

Found some time to dig into this on the train, and came away feeling that this will be extremely fragile. Supporting all of locale, weekdays, and weekStart is a fight against moment.js when we try not to modify global state. I think there are three options here:

  1. Fix this up and accept that we're still modifying global state, even though we're trying to avoid that.

  2. Wait for a feature like Implement locale inheritance and locale updating moment/moment#2774, which would effectively let us subclass the global locale with the modifications that we want.

  3. Remove the locale, weekdays, and weekStart options entirely, and defer to the global moment.js handling of these. I figure users will be localizing their moment configurations anyway, but this removes the ability to have picker-specific locale configuration. Then again, picker-specific locales seem a bit contrived to me.

@rafeememon rafeememon mentioned this pull request Dec 21, 2015
Merged
@martijnrusschen
Copy link
Member Author

Replaced by #277

@martijnrusschen martijnrusschen deleted the moment-for-week branch January 8, 2016 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use moment.weekdaysMin() for weekdays
2 participants
@martijnrusschen @rafeememon